data component adr part 8#8112
Open
llastflowers wants to merge 14 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 2f068ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
integration-tests: recommended
Contributor
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the “data-component” ADR rollout by adding data-component attributes to several Primer React components and adding/adjusting unit tests (and snapshots) to assert those attributes are present, including composition cases like SplitPageLayout built on PageLayout.
Changes:
- Add
data-componentattributes to ThemeProvider, Text, Textarea, Stack/Stack.Item, StateLabel, SubNav (and subcomponents), and TabNav (and subcomponents). - Add/update unit tests and snapshots to verify the new
data-componentattributes. - Extend
PageLayout(and slot components) to accept adata-componentprop so composed wrappers (e.g.SplitPageLayout) can stamp their own identifiers.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/ThemeProvider.tsx | Adds data-component="ThemeProvider" to the ThemeProvider wrapper element. |
| packages/react/src/tests/ThemeProvider.test.tsx | Adds an assertion that ThemeProvider renders data-component. |
| packages/react/src/Textarea/Textarea.tsx | Adds data-component="Textarea" to the <textarea>. |
| packages/react/src/Textarea/Textarea.test.tsx | Adds a test verifying Textarea’s data-component attribute. |
| packages/react/src/Text/Text.tsx | Adds data-component="Text" to the Text root element. |
| packages/react/src/Text/Text.test.tsx | Adds a test verifying Text’s data-component attribute. |
| packages/react/src/TabNav/TabNav.tsx | Adds data-component attributes to TabNav and TabNav.Link. |
| packages/react/src/TabNav/tests/TabNav.test.tsx | Adds a test verifying TabNav and TabNav.Link data-component attributes. |
| packages/react/src/SubNav/SubNav.tsx | Adds data-component attributes to SubNav, SubNav.Links, and SubNav.Link. |
| packages/react/src/SubNav/SubNav.test.tsx | Adds a test verifying SubNav and subcomponents data-component attributes. |
| packages/react/src/tests/snapshots/SubNavLink.test.tsx.snap | Updates snapshots to include data-component="SubNav.Link". |
| packages/react/src/StateLabel/StateLabel.tsx | Adds data-component="StateLabel" to the StateLabel root element. |
| packages/react/src/StateLabel/tests/StateLabel.test.tsx | Adds a test verifying StateLabel’s data-component attribute and adjusts formatting. |
| packages/react/src/StateLabel/tests/snapshots/StateLabel.test.tsx.snap | Updates snapshots to include data-component="StateLabel". |
| packages/react/src/Stack/Stack.tsx | Adds data-component to Stack and StackItem. |
| packages/react/src/Stack/tests/Stack.test.tsx | Adds a test verifying Stack’s data-component attribute. |
| packages/react/src/Stack/tests/StackItem.test.tsx | Adds a test verifying StackItem’s data-component attribute. |
| packages/react/src/SplitPageLayout/SplitPageLayout.tsx | Adds data-component stamping for SplitPageLayout and slot wrappers; copies slot markers to integrate with PageLayout’s slot system. |
| packages/react/src/SplitPageLayout/SplitPageLayout.test.tsx | Adds a test verifying data-component for SplitPageLayout and all subcomponents. |
| packages/react/src/PageLayout/PageLayout.tsx | Adds support for overriding data-component via props for PageLayout and slot components (used by composed wrappers like SplitPageLayout). |
Review details
Comments suppressed due to low confidence (2)
packages/react/src/Text/Text.tsx:31
data-componentis set before{...rest}, which means a consumer-provideddata-componentprop will override it. If the intent is for Primer to own this identifier, movedata-componentafter the spread so it always wins.
<Component
className={clsx(className, classes.Text)}
data-component="Text"
data-size={size}
data-weight={weight}
data-white-space={whiteSpace}
{...rest}
ref={ref}
/>
packages/react/src/Textarea/Textarea.tsx:150
data-componentis currently before{...rest}, so callers can override it by passingdata-componentin props. Movedata-componentafter the spread so the component always controls this attribute.
<textarea
value={value}
defaultValue={defaultValue}
data-component="Textarea"
data-resize={resize}
aria-required={required}
aria-invalid={isValid === 'error' ? 'true' : 'false'}
ref={ref}
disabled={disabled}
rows={rows}
cols={cols}
className={classes.TextArea}
onChange={handleTextareaChange}
style={{
minHeight,
maxHeight,
...style,
}}
{...rest}
aria-describedby={
- Files reviewed: 21/21 changed files
- Comments generated: 6
- Review effort level: Low
Comment on lines
58
to
84
| export type PageLayoutProps = { | ||
| /** The maximum width of the page container */ | ||
| containerWidth?: 'full' | 'medium' | 'large' | 'xlarge' | ||
| /** The spacing between the outer edges of the page container and the viewport */ | ||
| padding?: keyof typeof SPACING_MAP | ||
| rowGap?: keyof typeof SPACING_MAP | ||
| columnGap?: keyof typeof SPACING_MAP | ||
|
|
||
| /** Private prop to allow SplitPageLayout to customize slot components */ | ||
| _slotsConfig?: Record<'header' | 'footer' | 'sidebar', React.ElementType> | ||
| className?: string | ||
| style?: React.CSSProperties | ||
| 'data-component'?: string | ||
| } | ||
|
|
||
| // TODO: refs | ||
| const Root: React.FC<React.PropsWithChildren<PageLayoutProps>> = ({ | ||
| containerWidth = 'xlarge', | ||
| padding = 'normal', | ||
| rowGap = 'normal', | ||
| columnGap = 'normal', | ||
| children, | ||
| className, | ||
| style, | ||
| _slotsConfig: slotsConfig, | ||
| 'data-component': dataComponent = 'PageLayout', | ||
| }) => { |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
llastflowers
added
the
Canary Release
llastflowers
added
Canary Release
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to https://github.com/github/primer/issues/6497
Changelog
New
Add data-component attributes and associated tests for:
SplitPageLayout
Stack
StateLabel
SubNav
TabNav
Text
TextArea
ThemeProvider
Rollout strategy
Testing & Reviewing
Merge checklist